-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: handle bad callback in asyc_wrap #31946
Conversation
src/async_wrap-inl.h
Outdated
using v8::Name; | ||
using v8::String; | ||
using v8::Symbol; | ||
using v8::Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don’t add using
statements to headers (we should probably have a linter rule against that, at least for the v8 symbols…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax, grep "using v8::" src/*.h
I am seeing this pattern in few files already. If I remove using
phrase, is there any way I can elevate ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these other files should also not use it.
There are no shorthands in that case, we do spell out the entire type in headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax,thanks. Reverted the changes. What is the rationale for this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, why we want to skip for headers ? Is there any performance reason or something else ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HarshithaKP Because then the symbols are available in any source file that includes this header, even if that’s not obvious. For example, this code would fail to compile if example.h
has a using v8::Array;
line:
#include "example.h"
class Array {};
Align with the MaybeLocal<> API contract
2a91f94
to
fc53c8d
Compare
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
landed in 757e203 |
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Align with the MaybeLocal<> API contract PR-URL: #31946 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
src: handle bad callback in asyc_wrap
Align with the MaybeLocal<> API contract
Refs:
node/deps/v8/include/v8.h
Lines 345 to 349 in 9403250
addresses an @addaleax 's TODO
src: elevate v8 namespaces
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes